test: add import compliance tests #4537

JonathanOppenheimer · 2025-11-21T23:28:19Z

Why this should be merged

This PR accomplishes three things:

Prevent files inside of the vms/evm directory from importing packages within the graft/ directory.
- This is important because with the removal of coreth, there is not longer a natural boundary between the uplifted and non-uplifted code. We need to enforce such a boundary.
- This boundary is important, especially as other people individuals start work on properly uplfiting code post monorepo. Reviewers and developers may not be familiar with why we want this distinction to exist and programmatically enforcing this boundary will prevent mistakes and leakage of legacy EVM code into AvalancheGo.
Prevent libevm/pseudo from being imported anywhere in AvalancheGo

See this issue for additional context Remove libevm import restriction test from graft/ post merge. #4541

How this works

The boundaries are as follows:

graft/coreth can be imported everywhere in AvalancheGo except vms/evm.
- However, vms/evm/emulate is an exception to this rule
graft/subnet-evm can not be imported anywhere in AvalancheGo
- However, vms/evm/emulate is an exception to this rule

How this was tested

Existing CI. I also added a sample "bad" commit (404acd5) to show how CI would behave.

edit: this is no longer a good example and is out of date but I kept it to show the formatting -- pretend the file is in vms/evm/sample_bad.go.

Need to be documented in RELEASES.md?

No

tests/imports_test.go

graft/scripts/forbidden-geth-files.txt

graft/scripts/libevm-allowed-packages.txt

vms/evm/imports_test.go

vms/evm/scripts/libevm-allowed-packages.txt

vms/evm/imports_test.go

maru-ava · 2025-11-25T11:25:19Z

vms/evm/imports_test.go

+		isInEmulate := strings.Contains(path, "/vms/evm/emulate/")
+		isInVmsEvm := strings.Contains(path, "/vms/evm/")
+		isCoreth := strings.Contains(importPath, "/graft/coreth")
+		isSubnetEVM := strings.Contains(importPath, "/graft/subnet-evm")


(No action required) Not a biggie, but would prefer to see subnet-evm specific stuff as part of the migration not in advance.

But also, why is testing for these independent grafts conflated? Maybe implement 2 tests that use the same helpers, one for each graft location?

@ARR4N suggested that all of this be combined into a single import violation test. Would you still like to see the tests separated out? I see the benefit of both ways

single test: less code, everything in one place

two tests: separation makes more sense semantically

I could also do a single test with two subtests?

maru-ava · 2025-11-25T11:32:42Z

vms/evm/imports_test.go

+	require.Fail(t, formatImportViolations(foundImports, header))
+}
+
+// TestDoNotImportLibevmPseudo ensures that no code in the repository imports


(No action required) If it's libevm-internal only, why isn't the package called internal so that the restriction is enforced automatically by golang?

@ARR4N would this be possible?

vms/evm/imports_test.go

ARR4N · 2025-11-25T11:07:22Z

vms/evm/imports_test.go

+// - graft/coreth can be imported anywhere EXCEPT vms/evm (but vms/evm/emulate is an exception)
+// - graft/subnet-evm cannot be imported anywhere EXCEPT vms/evm/emulate


Why are these the rules? I understand emulate being allowed to import, but not the other limitations.

Rule of thumb for all code, not just this instance: comments should explain "why", not "what" nor "how".

I added a longer comment explaining why - is it satisfactory?

vms/evm/imports_test.go

ARR4N · 2025-11-25T12:25:19Z

I think the entirety of this PR can be reduced to something like this:

func TestImportViolations(t *testing.T) {
	const root ".."
	err := filepath.Walk(root, func(file string, info fs.FileInfo, err error) error {
		if err != nil {
			return err
		}
		if strings.ToLower(filepath.Ext(file)) != "go" {
			return nil
		}

		node, err := parser.ParseFile(token.NewFileSet(), file, nil, parser.ImportsOnly)
		if err != nil {
			return fmt.Errorf("parser.ParseFile(..., %q, ...): %v", file, err)
		}

		for _, spec := range node.Imports {
			if spec.Path == nil {
				continue
			}
			imp := strings.Trim(spec.Path.Value, `"`)

			inGraft := strings.Contains(file, "/graft/")
			inEmulate := strings.Contains(file, "/vms/evm/emulate/")
			inVMsEVM := strings.Contains(file, "/vms/evm/")
			importsPseudo := isPackageIn(imp, "github.com/ava-labs/libevm/libevm/pseudo")
			importsCoreth := isPackageIn(imp, "github.com/ava-labs/avalanchego/graft/coreth")
			importsSubnetEVM := isPackageIn(imp, "github.com/ava-labs/avalanchego/graft/subnet-evm")

			// TODO: before merging, this needs a comment explaining why each of them are a violation
			violations := []bool{
				importsPseudo,
				!inGraft && importsCoreth && inVMsEVM && !inEmulate,
				!inGraft && importsSubnetEVM && !inEmulate,
			}
			if slices.Contains(violations, true) {
				t.Errorf("File %q imports %q", file, imp)
			}
		}
		return nil
	})

	require.NoErrorf(t, err, "filepath.Walk(%q)", root)
}

func isPackageIn(importPath, root string) bool {
	if importPath == root {
		return true
	}
	if len(importPath) < len(root)+1 {
		return false
	}
	return strings.HasPrefix(importPath, root) && importPath[len(root)] == '/'
}

I'm not particularly happy with the relative path to walk nor the way of checking if something is in a sub-dir (they can be fixed together), but otherwise I think this covers all bases.

Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com> Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>

JonathanOppenheimer · 2025-12-01T18:46:40Z

I'm not particularly happy with the relative path to walk nor the way of checking if something is in a sub-dir (they can be fixed together), but otherwise I think this covers all bases.

I feel bad that you put all of these great comments together and then just gave me a much simpler test, but your diligence is appreciated. I have replaced the file with your suggested test (tweaked slightly)

stale

vms/evm/imports_test.go

ARR4N

GitHub is forcing me to add a comment here for no reason (well, no reason beyond that it's a terrible product).

vms/evm/imports_test.go

ARR4N · 2025-12-02T09:52:14Z

vms/evm/imports_test.go

+// TODO(jonathanoppenheimer): remove the emulate functionality once the emulate package is removed.
+// TODO(jonathanoppenheimer): remove the graft functionality once the graft package will be removed.
+func TestImportViolations(t *testing.T) {
+	var violations []string


Why add the layer of indirection? Just report the violation when it arises.

FWIW this is why forcing use of require instead of judicious use of assert/require as appropriate is a bad idea. If multiple violations occur then it forces whack-a-mole fixing and rerunning of the test every time. Strict adherence to a style guide is not a valid reason to write more complicated code as it's antithetical to the purpose of a guide.

assert was banned because in too many instances it was assumed to be equivalent to require (fail-fast) and this resulted in buggy tests. For those instances where multiple violations are possible, t.Log is always an option.

This also will catch all the violations no? Fill up the array first, and then check the array at the end -- so it won't have to be rerun a ton of times.

I assume he would prefer that the use of inline assertions so as to avoid the requirement to accumulate failures and output them at the end?

FWIW, I'm sympathetic to revisiting the assert ban, maybe something to bring up in retro? I would hope that we now have enough review maturity to avoid misuse going forward.

Yah, I understand. If we'd like to revisiting the ban, that totally makes sense, as it would certainly enable additional testing patterns, but I don't think this is the PR to do that in 😂. For retro!

ARR4N · 2025-12-02T09:56:20Z

vms/evm/imports_test.go

+			inGraft := strings.Contains(file, "/graft")
+			inEmulate := strings.Contains(file, "/vms/evm/emulate")
+			inVMsEVM := strings.Contains(file, "/vms/evm")


strings.Contains is subject to false positives. First find the repo root with filepath.Abs(root) and then check for prefixes with strings.HasPrefix(file, filepath.Join(repoRoot, "/graft")) etc.

I've done so!

Let me know if this what you had in mind.

vms/evm/imports_test.go

ARR4N · 2025-12-02T10:00:53Z

vms/evm/imports_test.go

+	// Check if importPath is a subpackage by ensuring it has the targetedImport prefix
+	// AND the next character is '/'. This is to prevent false positives where one
+	// package name is a prefix of another like "github.com/foo" vs "github.com/foobar".


Suggested change

// Check if importPath is a subpackage by ensuring it has the targetedImport prefix

// AND the next character is '/'. This is to prevent false positives where one

// package name is a prefix of another like "github.com/foo" vs "github.com/foobar".

This just says what the code does so shouldn't be there as it's redundant and risks being or becoming incorrect. Heuristic: only explain "why" in inline comments, not how nor what. If a how or what is necessary then the code probably needs to be refactored.

More details.

I think the what is necessary as Maru requested an explanation of the intent behind the code here: #4537 (comment).

Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com> Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>

test: add import compliance tests

21b5aea

JonathanOppenheimer requested a review from alarso16 November 21, 2025 23:28

JonathanOppenheimer self-assigned this Nov 21, 2025

JonathanOppenheimer added the testing This primarily focuses on testing label Nov 21, 2025

github-project-automation bot added this to avalanchego Nov 21, 2025

JonathanOppenheimer added 3 commits November 21, 2025 18:35

test: tweak violation wording

7e33965

test: add violation to show failure example

404acd5

test: remove violation

dec282e

JonathanOppenheimer marked this pull request as ready for review November 21, 2025 23:37

JonathanOppenheimer requested review from joshua-kim and maru-ava as code owners November 21, 2025 23:37

JonathanOppenheimer commented Nov 21, 2025

View reviewed changes

tests/imports_test.go Show resolved Hide resolved

JonathanOppenheimer added 6 commits November 21, 2025 18:50

test: only enforce on vms/evm directory

234a039

test: clarify boundaries

9a39d6b

test: rename test

5017e1e

test: clarify comments

e490151

chore: lint

f411817

test: add license violation test

84fbb35

JonathanOppenheimer requested a review from StephenButtolph as a code owner November 22, 2025 00:50

JonathanOppenheimer added 3 commits November 21, 2025 20:10

test: inline once used type

498915c

tests: rename libevm test

f4fbfac

chore: fix typo

69e8354

JonathanOppenheimer commented Nov 22, 2025

View reviewed changes

graft/scripts/forbidden-geth-files.txt Outdated Show resolved Hide resolved

JonathanOppenheimer added 2 commits November 24, 2025 13:57

test: remove licensing test (excessive)

be5228a

Merge branch 'master' into JonathanOppenheimer/import-testing

d607b78

maru-ava reviewed Nov 24, 2025

View reviewed changes

graft/scripts/libevm-allowed-packages.txt Outdated Show resolved Hide resolved

maru-ava reviewed Nov 24, 2025

View reviewed changes

vms/evm/imports_test.go Show resolved Hide resolved

test: check both evm code locations

99ed2ce

JonathanOppenheimer commented Nov 24, 2025

View reviewed changes

vms/evm/scripts/libevm-allowed-packages.txt Outdated Show resolved Hide resolved

test: move import tests to evm directory

e9cc48b

test: reconsider boundary tests

cbcd663

JonathanOppenheimer added the monorepo label Nov 24, 2025

JonathanOppenheimer marked this pull request as ready for review November 24, 2025 23:06

maru-ava reviewed Nov 25, 2025

View reviewed changes

ARR4N previously requested changes Nov 25, 2025

View reviewed changes

github-project-automation bot moved this to In Progress 🏗️ in avalanchego Nov 25, 2025

ARR4N reviewed Nov 25, 2025

View reviewed changes

vms/evm/imports_test.go Outdated Show resolved Hide resolved

github-actions bot removed the monorepo label Nov 25, 2025

JonathanOppenheimer added the monorepo label Dec 1, 2025

JonathanOppenheimer and others added 2 commits December 1, 2025 13:31

Update vms/evm/imports_test.go

f482da4

Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com> Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>

test: use Arran's rewritten test

c4b31fc

JonathanOppenheimer added 3 commits December 1, 2025 13:48

fix: add .go

7d513d4

Merge branch 'master' into JonathanOppenheimer/import-testing

40e5904

chore: lint

b6541d1

JonathanOppenheimer requested review from ARR4N and maru-ava December 1, 2025 19:12

chore: move comment

70f33bc

maru-ava reviewed Dec 1, 2025

View reviewed changes

docs: document functions

57f083c

JonathanOppenheimer requested a review from maru-ava December 1, 2025 22:09

ARR4N reviewed Dec 2, 2025

View reviewed changes

JonathanOppenheimer and others added 4 commits December 2, 2025 12:08

Update vms/evm/imports_test.go

63a88d9

Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com> Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>

Update vms/evm/imports_test.go

249e7cd

Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com> Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>

Update vms/evm/imports_test.go

fee0356

Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com> Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>

test: use absolute file paths

d24963b

JonathanOppenheimer requested a review from ARR4N December 2, 2025 17:20

JonathanOppenheimer mentioned this pull request Dec 2, 2025

Remove old EVM import testing #4643

Open

		// - graft/coreth can be imported anywhere EXCEPT vms/evm (but vms/evm/emulate is an exception)
		// - graft/subnet-evm cannot be imported anywhere EXCEPT vms/evm/emulate

	// Check if importPath is a subpackage by ensuring it has the targetedImport prefix
	// AND the next character is '/'. This is to prevent false positives where one
	// package name is a prefix of another like "github.com/foo" vs "github.com/foobar".

test: add import compliance tests #4537

Are you sure you want to change the base?

test: add import compliance tests #4537

Uh oh!

Conversation

JonathanOppenheimer commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Need to be documented in RELEASES.md?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ARR4N commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonathanOppenheimer commented Dec 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ARR4N left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maru-ava Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathanOppenheimer Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

JonathanOppenheimer commented Nov 21, 2025 •

edited

Loading

ARR4N commented Nov 25, 2025 •

edited

Loading

maru-ava Dec 2, 2025 •

edited

Loading

JonathanOppenheimer Dec 2, 2025 •

edited

Loading